Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADD: a basic BTM N-trace spec compliant trace encoder model #1824

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

iansseijelly
Copy link

This PR adds a trace encoder model to spike, making it capable of dumping encoded n-trace packets in the slow path, which a separate decoder can later decode. It is enabled by a trace controller device (see: ucb-bar/spike-devices#8) and implements the basic trace encoding mechanisms. Adding this to spike will make the software development of the decoder easier, and a reference for RTL encoders to check upon.

  • Known issues: adding a separate command trigger may be a good idea.

riscv/processor.h Outdated Show resolved Hide resolved
@aswaterman
Copy link
Collaborator

@YenHaoChen can you review this one?

riscv/execute.cc Outdated Show resolved Hide resolved
riscv/execute.cc Outdated Show resolved Hide resolved
@iansseijelly iansseijelly marked this pull request as draft September 30, 2024 21:13
@iansseijelly iansseijelly marked this pull request as ready for review October 2, 2024 19:41
@iansseijelly
Copy link
Author

Hi, thanks for the review. I have fixed (I think) the memory leak regarding trace_encoder_push_commit(). I have also, using this PR + the controller device + a simple decoder implementation (https://github.com/iansseijelly/rust-trace-deocder), decoded a hello world trace correctly. Please let me know what are the future things to fix and add.

riscv/trace_encoder_n.cc Outdated Show resolved Hide resolved
riscv/trace_encoder_n.cc Outdated Show resolved Hide resolved
riscv/execute.cc Outdated Show resolved Hide resolved
#include "trace_encoder_n.h"

trace_encoder_n::trace_encoder_n() {
this->trace_sink= fopen("trace_n.bin", "wb");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file name should be different for each hartId .. a simple way to avoid clobbering the same file in a multi-core with-trace simulation.

.i_type = _get_insn_type(&fetch.insn, npc != p->get_state()->pc + insn_length(fetch.insn.bits())),
.exc_cause = 0,
.tval = 0,
.priv = P_M, // TODO: check for processor privilege level
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This priv should be fixed.

Should this trace_encoder_push_commit also be added to the take_trap and take_interrupt methods?

riscv/processor.cc Outdated Show resolved Hide resolved
public:

trace_encoder_n() {
this->trace_sink= fopen("trace_n.bin", "wb");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the constructor should include hartId as a parameter, and then trace_n.bin can be hart<id>_trace_n.bin here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should implement a destructor for this class that fcloses the file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like multi-core needs to be handled more carefully. According to the spec there needs to be a spec funnel, and the decoder also needs to spawn multiple workers and discern sources. Maybe we should add the multi-core support later.

riscv/trace_encoder_n.h Outdated Show resolved Hide resolved
riscv/trace_encoder_n.h Outdated Show resolved Hide resolved
riscv/trace_ingress.cc Outdated Show resolved Hide resolved
riscv/trace_encoder_n.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants